Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EstimateTradeBasedOnPriceImpact Query #6167

Merged
merged 112 commits into from
Sep 29, 2023

Conversation

VitalyV1337
Copy link
Contributor

@VitalyV1337 VitalyV1337 commented Aug 25, 2023

What is the purpose of the change

Add a description of the overall background and high level changes that this PR introduces

Added an EstimateTradeBasedOnPriceImpact query to the x/poolmanager/query_proto_wrap.go, the goal of this query is to help external blockchains/smart-contracts to fill in the MsgSwapExactAmountIn details by retrieving input and output values for a trade. Given a total input amount, a maximum price impact, and an external price reference this query should return an input and output trade that respects the price impact of a pool.

This query operates in a binary search way where inputs are recursively tried and reduced each time if the price impact of the output is not met. If no possible trade is found then the outputs would be zero.

Testing and Verifying

This change added tests and can be verified as follows:

  • *Added unit test that validates the query reacts as needed for the Balancer/StableSwap and Concentrated liquidity pool types under different input conditions.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

mergify bot and others added 30 commits June 22, 2023 11:48
…5600) (osmosis-labs#5607)

* add user-pool position breakdown for superfluid

* fix godoc

* add basic test

* slightly expand test

* remove defense in depth

* use osmoutils

* error with locked but non superfluid positions

(cherry picked from commit bab837c)

Co-authored-by: Adam Tucker <[email protected]>
…abs#5619)

* Paralleze go tests

* Speculative execution of go-split-test-files to save time

* Revert "Speculative execution of go-split-test-files to save time"

This reverts commit ca5ea6f.

(cherry picked from commit 35f54cb)

Co-authored-by: Niccolo Raspa <[email protected]>
…bs#5622)

* Remove double caching from test github action

* Remove double caching from build github action + separate go mod download

* Update get-diff patterns in build action

* remove duplicate caching in update-go-import github action

* Rename github action

* remove duplicate caching in test-e2e-makefile

* remove duplicate caching in lint

(cherry picked from commit 24f580d)

Co-authored-by: Niccolo Raspa <[email protected]>
…mosis-labs#5629)

* Remove proto deserialization from HasPosition

* regenerate go mod

---------

Co-authored-by: alpo <[email protected]>
(cherry picked from commit 7df187a)

Co-authored-by: Dev Ojha <[email protected]>
…osmosis-labs#5632)

* backporting adding errors to logs

* hardcoded string as const

(cherry picked from commit 80bf16e)

Co-authored-by: Nicolas Lara <[email protected]>
…abs#5640)

* upgrade handler dai amount change

* update comment

* fix test

* update error margin

* 73 cent spot price

* update e2e json

* update init config

* just need v15 init image now

* update v15 init image

(cherry picked from commit e9f0b1e)

Co-authored-by: Adam Tucker <[email protected]>
…abs#5651)

Closes: #XXX

## What is the purpose of the change
This PR includes the final changes from audit results

## Testing and Verifying
Ensured that existing test cases pass
## Documentation and Release Note

  - [ ] Does this pull request introduce a new feature or user-facing behavior changes?
  - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`?

Where is the change documented?
  - [ ] Specification (`x/{module}/README.md`)
  - [ ] Osmosis documentation site
  - [ ] Code comments?
  - [ ] N/A

(cherry picked from commit ed8ed77)

Co-authored-by: Matt, Park <[email protected]>
…osis-labs#5650) (osmosis-labs#5652)

* Reduce clmath test compilation time, fix discrepancy in rounding

* remove unneeded helper

(cherry picked from commit 9f7b70b)

Co-authored-by: Dev Ojha <[email protected]>
* modify upgrade handler

* update test

* fund community pool 1dai before upgrade e2e

* fund chain b as well

* test added

(cherry picked from commit 6138de4)

Co-authored-by: Adam Tucker <[email protected]>
* update test script deps

* another update

(cherry picked from commit 13cb7e7)

Co-authored-by: Jon Ator <[email protected]>
* Fix testing errors in tick tests

* Fix remaining case

* generate more test vectors rather than hardcode

* delete more unneeded lines

(cherry picked from commit d7336b5)

Co-authored-by: Dev Ojha <[email protected]>
…osis-labs#5669)

* check if lock is mature in sf query

* check if lock has matured in query

* Update position.go

* simpler change

(cherry picked from commit 0b7700a)

Co-authored-by: Adam Tucker <[email protected]>
(cherry picked from commit 0a867a8)

Co-authored-by: Adam Tucker <[email protected]>
osmosis-labs#5689)

* Introduce ResetTest, use in CL for 2x test speedup

* now 3.8 seconds

* remove extraneous setupTest calls

* reduce gamm test time

* speedup more of gamm

* driveby invariant test speedup

* remove extra println

* undo ResetTest being a separate fn

* same dev UX improvement to gamm

* Add comments

(cherry picked from commit 28a12d0)

Co-authored-by: Dev Ojha <[email protected]>
…labs#5688)

* repro infinite loop in swap logic

* refactor/fix: swap step iteration limit

* Update x/concentrated-liquidity/swaps_test.go

Co-authored-by: Adam Tucker <[email protected]>

* swap limit to 100

* clearer comments

* Update x/concentrated-liquidity/swaps.go

Co-authored-by: alpo <[email protected]>

* Update x/concentrated-liquidity/types/errors.go

Co-authored-by: alpo <[email protected]>

---------

Co-authored-by: alpo <[email protected]>
Co-authored-by: Adam Tucker <[email protected]>
Co-authored-by: alpo <[email protected]>
(cherry picked from commit a593f6a)

Co-authored-by: Roman <[email protected]>
…ngs (osmosis-labs#5617) (osmosis-labs#5667)

* Driveby apptesting changes I made, while looking at other testing things

* undo one change

* Fix failing test

* Fix gamm test

(cherry picked from commit 1b317e4)

Co-authored-by: Dev Ojha <[email protected]>
…smosis-labs#5709)

* query total liquidity across all cl pools

* add total liquidity across all three pool types

* query gen

* query gen

* use O(n) time total liquidity query

* add store entry

* no need to store total liq in genesis

(cherry picked from commit e44b5cd)

Co-authored-by: Adam Tucker <[email protected]>
…#5711)

* cl unbonding positions query

* cl unbonding positions query

* regen proto

* add basic test

* chore: sf undelegating / delegated cl queries (osmosis-labs#5691)

* undelegating / delegated cl queries

* correct comment

* remove switch and add errors.Is

* Update proto/osmosis/superfluid/query.proto

Co-authored-by: Jon Ator <[email protected]>

* Update proto/osmosis/superfluid/query.proto

Co-authored-by: Jon Ator <[email protected]>

* rename to delegate in place of bond

* fix errors Is

---------

Co-authored-by: Jon Ator <[email protected]>

* remove switch statement

* add position id to error

* add synth lock to q

* regen proto

---------

Co-authored-by: Jon Ator <[email protected]>
(cherry picked from commit f9b3cc1)

Co-authored-by: Adam Tucker <[email protected]>
…smosis-labs#5712)

* repro infinite loop in swap logic

* precision increase solution to infinite loop bug

* fix

* remove logic

* remove error

* updates

* updates

* updates

* updates

* begin switching zeroForOneStrategy.ComputeSwapWithinBucketInGivenOut

* switch remaining zero for one swap step and add rounding comments

* begin switching cur sqrt price to big dec in swaps

* try adding CalculateSqrtPriceToTickBigDec

* one for zero out given in tests

* fix one for zero tests

* update one for zero tests

* in-progress test

* updates

* fix TestComputeSwapStepOutGivenIn_ZeroForOne

* fix TestComputeSwapStepInGivenOut_ZeroForOne

* convert current sqrt price to BigDec and resolve syntax errors

* updates

* clean up

* clean ups

* clean ups

* fix TestComputeSwapState_Inverse

* comment

* updates

* clean ups

* remove unused function

* remove unused GetNextSqrtPriceFromAmount1OutRoundingDown

* switch GetNextSqrtPriceFromAmount0InRoundingUp to big dec

* fully convert GetNextSqrtPriceFromAmount0OutRoundingUp to big dec

* clean up math tests more

* more math test clean up

* fully convert calc methods to big dec

* fix model package tests

* fix TestAddToPosition

* fix TestCalculateSpotPrice

* fix TestSingleSidedAddToPosition

* fix TestUninitializePool

* go mod update

* big dec comparison correction and prints

* fix equality issues

* convert tests to big dec

* fix a few swap tests

* small e2e fix

* more swap test fixes

* updates

* fix another test

* please get fixed

* fix all out given in swap tests

* e2e

* fix all one for zero in given out

* I can fix you

* clean ups

* MulRoundUp test

* test SDKDecRoundUp

* clean up

* revert launch.json

* comment

* add zero for one swap strategy ULP tests

* typo

* remove errors

* remove prints

---------

Co-authored-by: alpo <[email protected]>
(cherry picked from commit 950fd12)

Co-authored-by: Roman <[email protected]>
osmosis-labs#5687) (osmosis-labs#5692)

* Add lock reward receiver

* Fix typo

(cherry picked from commit d5fcd08)

Co-authored-by: Matt, Park <[email protected]>
…me ULP (osmosis-labs#5693) (osmosis-labs#5713)

* repro infinite loop in swap logic

* precision increase solution to infinite loop bug

* fix

* remove logic

* remove error

* updates

* updates

* updates

* updates

* begin switching zeroForOneStrategy.ComputeSwapWithinBucketInGivenOut

* switch remaining zero for one swap step and add rounding comments

* begin switching cur sqrt price to big dec in swaps

* try adding CalculateSqrtPriceToTickBigDec

* one for zero out given in tests

* fix one for zero tests

* update one for zero tests

* in-progress test

* updates

* fix TestComputeSwapStepOutGivenIn_ZeroForOne

* fix TestComputeSwapStepInGivenOut_ZeroForOne

* convert current sqrt price to BigDec and resolve syntax errors

* updates

* clean up

* clean ups

* clean ups

* fix TestComputeSwapState_Inverse

* comment

* updates

* clean ups

* remove unused function

* remove unused GetNextSqrtPriceFromAmount1OutRoundingDown

* switch GetNextSqrtPriceFromAmount0InRoundingUp to big dec

* fully convert GetNextSqrtPriceFromAmount0OutRoundingUp to big dec

* clean up math tests more

* more math test clean up

* fully convert calc methods to big dec

* fix model package tests

* fix TestAddToPosition

* fix TestCalculateSpotPrice

* fix TestSingleSidedAddToPosition

* fix TestUninitializePool

* go mod update

* big dec comparison correction and prints

* fix equality issues

* convert tests to big dec

* fix a few swap tests

* small e2e fix

* more swap test fixes

* updates

* fix another test

* please get fixed

* fix all out given in swap tests

* e2e

* fix all one for zero in given out

* I can fix you

* clean ups

* MulRoundUp test

* test SDKDecRoundUp

* clean up

* revert launch.json

* comment

* refactor/security: prevent same execution price exploit within the same ULP

* add zero for one swap strategy ULP tests

* typo

* remove errors

* remove prints

* typos

* typo

---------

Co-authored-by: alpo <[email protected]>
(cherry picked from commit cf223be)

Co-authored-by: Roman <[email protected]>
@mattverse
Copy link
Member

@VitalyVolozhinov Can you take a look at the branching here? Seems like there are conflicts that must be resolved before merging 👀

# Conflicts:
#	x/poolmanager/client/cli/query.go
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! This would definitely be helpful for so much people, thanks for the contribution!

Before we merge, could we do two following things:

  1. Add proto doc + docs in other places about invalid estimation returning 0 instead of erroring
  2. Have manual QA of the query being added? (cli or lcd does not matter)

Once this is done this should be good to go. Again, thanks so much for this 🔥

@VitalyV1337
Copy link
Contributor Author

Nice work! This would definitely be helpful for so much people, thanks for the contribution!

Before we merge, could we do two following things:

  1. Add proto doc + docs in other places about invalid estimation returning 0 instead of erroring
  2. Have manual QA of the query being added? (cli or lcd does not matter)

Once this is done this should be good to go. Again, thanks so much for this 🔥

I think the new changes may have broken the localnet and I cannot run it but I have previously tested the cli on CL pools doing the following:

Running the code using local-net commands:

  • make localnet-init
  • make localnet-start
  • make localnet-keys
  • make localnet-cl-create-pool
  • make localnet-cl-create-positions

We then query the pool with various inputs to see what the responses are using the following command:

osmosisd query poolmanager estimate-trade-amount-in-amount-out-based-on-price-impact 10000000000000000uosmo uusdc 1 0.001 2.49

image

We can see as increase the input of uosmo it gives us appropriate input and output amounts to trade under the given slippage and twap value.

@mattverse
Copy link
Member

@VitalyVolozhinov Thanks so much for this <3

@mattverse mattverse closed this Sep 21, 2023
@mattverse mattverse reopened this Sep 21, 2023
@mattverse
Copy link
Member

@VitalyVolozhinov Can you run make proto-all + solve linter issues please?

@VitalyV1337
Copy link
Contributor Author

@VitalyVolozhinov Can you run make proto-all + solve linter issues please?

All issues should be solved

@migueldingli1997
Copy link
Contributor

Looking forward to seeing this merged 💪

@VitalyV1337
Copy link
Contributor Author

VitalyV1337 commented Sep 25, 2023

@mattverse Not sure why the proto file generation failed it had passed on our side: https://github.com/entrypoint-zone/osmosis/actions/runs/6296358703/job/17091183632

# Conflicts:
#	x/poolmanager/client/cli/query.go
@mattverse
Copy link
Member

@VitalyVolozhinov Did you try generating proto after merging with the latest main? Protobuf gen would often times fail when generated without having the branch merged with latest main, could be the problem 🤔 Try and lmk! If it doesn't seem to work, happy to merge for now and then re-protogen myself

@VitalyV1337
Copy link
Contributor Author

@VitalyVolozhinov Did you try generating proto after merging with the latest main? Protobuf gen would often times fail when generated without having the branch merged with latest main, could be the problem 🤔 Try and lmk! If it doesn't seem to work, happy to merge for now and then re-protogen myself

Yes, and it works on our side: https://github.com/entrypoint-zone/osmosis/actions/runs/6348615589/job/17245570892 not sure what is happening

@mattverse mattverse merged commit d387e22 into osmosis-labs:main Sep 29, 2023
1 check passed
@jasbanza
Copy link

Hi @mattverse, this looks amazing. When will it be available for CLI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:docs Improvements or additions to documentation C:x/poolmanager V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants